Conversation
| run: | | ||
| if [ -f pr_number.txt ]; then | ||
| echo "PR_NUMBER=$(cat pr_number.txt)" >> $GITHUB_ENV | ||
| else | ||
| echo "pr_number.txt not found, skipping comment." | ||
| exit 0 | ||
| fi |
Check failure
Code scanning / CodeQL
Environment variable built from user-controlled sources Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, the fix is to prevent arbitrary content from pr_number.txt from being written directly into the GITHUB_ENV file. We should (1) strip newlines so the variable assignment cannot be split across multiple lines, and (2) optionally validate that the value is a numeric PR number before using it. This ensures an attacker cannot inject additional environment variables by crafting pr_number.txt with embedded newlines or KEY=VALUE lines.
The best minimal fix here, without changing the workflow’s overall behavior, is to read pr_number.txt into a shell variable, normalize it to a single line, validate that it looks like a PR number (digits only), and then write it to $GITHUB_ENV using the hardened echo pattern recommended by GitHub: echo "VAR=$(echo "$VAR" | tr -d '\n')" >> "$GITHUB_ENV". If validation fails, the step should fail early rather than proceeding with a malicious or malformed value.
Concretely, in .github/workflows/pr-test-summary.yml lines 27–33, replace the simple cat-and-echo sequence with a small shell script that:
- Checks the file exists (as now).
- Reads
PR_NUMBERfrom the file. - Strips whitespace/newlines.
- Verifies it matches
^[0-9]+$. - Writes
PR_NUMBER=<sanitized_value>to$GITHUB_ENVusing double quotes around$GITHUB_ENV.
No external dependencies are needed; we can use standard POSIX tools (tr, grep/shell pattern) available in ubuntu-latest.
| @@ -26,7 +26,13 @@ | ||
| - name: Read PR number | ||
| run: | | ||
| if [ -f pr_number.txt ]; then | ||
| echo "PR_NUMBER=$(cat pr_number.txt)" >> $GITHUB_ENV | ||
| PR_NUMBER=$(cat pr_number.txt | tr -d '\r\n') | ||
| # Ensure PR_NUMBER consists only of digits to avoid environment injection | ||
| if ! printf '%s\n' "$PR_NUMBER" | grep -Eq '^[0-9]+$'; then | ||
| echo "Invalid PR number in pr_number.txt: '$PR_NUMBER'" | ||
| exit 1 | ||
| fi | ||
| echo "PR_NUMBER=$PR_NUMBER" >> "$GITHUB_ENV" | ||
| else | ||
| echo "pr_number.txt not found, skipping comment." | ||
| exit 0 |
e85f700 to
0956d5d
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors how pytest coverage gets reported back to pull requests by persisting test outputs as an artifact in the main test workflow and adding a separate workflow_run workflow to post the coverage comment after the run completes.
Changes:
- Update
test-build-idf-apps.ymlto save the PR number and upload pytest outputs as atest-resultsartifact. - Add
pr-test-summary.ymlto download the artifact onworkflow_runcompletion and post the coverage comment. - Minor YAML formatting tweaks (quote style and matrix formatting).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| .github/workflows/test-build-idf-apps.yml | Uploads PR-associated test outputs (coverage + junit xml + PR number) as an artifact instead of commenting inline. |
| .github/workflows/pr-test-summary.yml | New workflow that runs on completed upstream workflows, downloads the artifact, and comments coverage back to the PR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| on: | ||
| workflow_run: | ||
| workflows: | ||
| - Test Build IDF Apps | ||
| types: | ||
| - completed |
There was a problem hiding this comment.
This workflow triggers on all completed runs of “Test Build IDF Apps”, including push runs on main. Since the artifact is only uploaded for pull_request runs, the download/comment steps will fail for push-triggered runs. Add a guard (e.g. job/step if: github.event.workflow_run.event == 'pull_request') or otherwise skip when the upstream run wasn’t a PR.
1ca6e7e to
be8d2a7
Compare
be8d2a7 to
551bcf6
Compare
No description provided.